Skip to content

CROSSLINK-210 Add more search parameters#434

Merged
JanisSaldabols merged 18 commits intomainfrom
CROSSLINK-210
Mar 20, 2026
Merged

CROSSLINK-210 Add more search parameters#434
JanisSaldabols merged 18 commits intomainfrom
CROSSLINK-210

Conversation

@JanisSaldabols
Copy link
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings March 5, 2026 12:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds additional patron request search/filter capabilities by extending the patron_request schema and introducing a search view used by the list endpoint/CQL filtering. Also tightens supplier/requester lookup to include “side” and exposes the new needsAttention field through the API.

Changes:

  • Add needs_attention column and introduce patron_request_search_view with derived search fields.
  • Switch patron request listing to read from the new view and expand supported CQL fields.
  • Update GetPatronRequestBySupplierSymbolAndRequesterReqId to include side, and propagate through service/tests; expose needsAttention in API responses.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
broker/test/patron_request/api/api-handler_test.go Validates API returns default needsAttention=false; updates repo lookup call to include side.
broker/sqlc/pr_schema.sql Adds needs_attention and defines patron_request_search_view used for searching.
broker/sqlc/pr_query.sql Lists patron requests from the new view; adds needs_attention to create/update; adds side constraint to supplier+requesterReqId lookup.
broker/patron_request/service/message-handler.go Updates supplier-side lookups to include SideLending.
broker/patron_request/service/action_test.go Updates mock repo method signature to include side.
broker/patron_request/db/prrepo.go Adapts list scanning/mapping to new list row shape; adds side parameter to lookup.
broker/patron_request/db/prcql.go Adds additional CQL fields for filtering patron requests.
broker/patron_request/api/api-handler.go Exposes needsAttention in API responses; ensures RequesterReqID is set on create.
broker/oapi/open-api.yaml Documents new needsAttention field and CQL-filterable fields on GET /patron_requests.
broker/migrations/019_add_attention_field.up.sql Migration adds needs_attention and creates the search view.
broker/migrations/019_add_attention_field.down.sql Migration rollback drops the column and view.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.


You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.


You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.


You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.


You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.


You can also share your feedback on Copilot code review. Take the survey.

Copy link
Contributor

@jakub-id jakub-id left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-action failures still bypass the new needs_attention flag.

finalizeActionExecution() only calls setNeedsAttention() after RunAutoActionsOnStateEntry() returns successfully, but it returns early on auto-action error:

if stateChanged {
    err := a.RunAutoActionsOnStateEntry(ctx, updatedPr, &event.ID)
    if err != nil {
        return events.LogErrorAndReturnResult(ctx, "failed to execute auto action", err)
    }
}

if execResult.outcome == ActionOutcomeFailure && !updatedPr.NeedsAttention {
    a.setNeedsAttention(ctx, updatedPr)
}

That means a request can transition successfully, hit an auto action from the state model (NEW -> validate, supplier VALIDATED -> will-supply, etc.), fail there, and return an error without ever being marked for attention.

I think this is a behavioral gap relative to the new feature, since auto-triggered actions are still actions and their failures are exactly the kind of case an operator would need surfaced.

# Conflicts:
#	broker/oapi/open-api.yaml
#	broker/patron_request/api/api-handler.go
#	broker/patron_request/db/prcql.go
#	broker/sqlc/pr_query.sql
#	broker/sqlc/pr_schema.sql
@JanisSaldabols JanisSaldabols requested a review from jakub-id March 20, 2026 12:16
@jakub-id
Copy link
Contributor

@JanisSaldabols can you drop go.work.sum from this PR?

@JanisSaldabols JanisSaldabols merged commit 571b78f into main Mar 20, 2026
5 checks passed
@JanisSaldabols JanisSaldabols deleted the CROSSLINK-210 branch March 20, 2026 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants